-
Notifications
You must be signed in to change notification settings - Fork 5
(#1876, #2086) Create ncids cdns #2055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Viewing Information |
1 similar comment
Viewing Information |
59a98fa to
fd6c268
Compare
8ce9801 to
257c8c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces multiple distribution variants for the NCIDS CSS and JavaScript packages, creating three different builds: minimal, standard, and full versions for CDN distribution. The changes replace the previous Babel/Rollup configuration with a new build system that generates multiple outputs for different use cases.
- Replaces previous build system with new Rollup configuration for multiple JavaScript builds
- Creates three CSS variants (minimal, full, standard) and corresponding JavaScript bundles
- Adds CDN packaging and deployment automation through GitHub Actions
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ncids-js/rollup.config.mjs | New Rollup configuration generating UMD and ESM bundles for three variants |
| packages/ncids-css/*.scss | New entry points for minimal and full CSS variants |
| packages/ncids-css/packages/ncids-*/_index.scss | Package definitions for different NCIDS variants |
| scripts/zip-builds.sh | Script to package CDN distributions |
| .github/workflows/workflow.yml | GitHub Actions workflow for CDN build and deployment |
| testing/ncids-css-testing/src/stories/**/*.scss | Updated imports to use new variant names |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a9239c4 to
c9d1ce5
Compare
bryanpizzillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round, but it is close to being there.
|
|
||
| // NCIDS | ||
| // ------------------------------------- | ||
| @forward 'nci-autocomplete'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pull in minimal into full so we say that we don't have to worry about keeping things in sync.
Also can you forward all the packages we have and don't rely on packages forwarding others. e.g., add nci-back-to-top even though nci-footer imports it. Because today nci-footer imports it, but a pr could go in changing that, and then we would have to know to go here to add it. (and we will most likely forget)
| @@ -0,0 +1,13 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be auto inits so that someone can just add thse to their page. I think we should exclude anything without a normal autoinit and make a backlog for it.
| * - Footer | ||
| */ | ||
|
|
||
| export * from 'src/components/nci-header'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be auto inits so that someone can just add thse to their page.
e55bc2a to
1f65e79
Compare
5a4bda9 to
646a334
Compare
3e63950 to
a86a921
Compare
a86a921 to
1a1c385
Compare
bryanpizzillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The utility classes seem to be missing from the documentation site. I noticed it with the version bar, but all the utility pages are broken.
ff1ea3e to
70b45ed
Compare
481fb84 to
4c8c38e
Compare
|
I can't approve my own PR, but after talking to Bryan last night and looking over all the commits, lgtm (after merging commits) |
|
Per @blilianyu's testing on 12/17: The spanish language toggle has a special characters issue
|
blilianyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passes for design cc: @andyvanavery31
- Added tslib as a dependency for ncids-js as it is a peer dep for rollup typescript. - Updated ncids-css to use vite instead of webpack because it works. There are some plugins for postcss that we could have used, but we know vite works because we use it for storybook. - Setup js example site such that it allows for the use of the built files in ncids-js and ncids-css. Without making super major changes this does require one to make sure they are building those packages before running the test site. In a perfect scenario the ncids-js and ncids-css dependencies would be built before the ncids-js-testing package. - I did update the prepare for ncids-css to also build. This should make sure there is at least some sort of css for ncids-js-testing. ncids-js already builds on prepare. Closes #1876
ddca8ba to
dce48d3
Compare

Closes #1876
Closes #2086
Pull Request Details
Add description
Closes #
Author PR Checklist
Items that the author of the PR is responsible for checking before submitted the PR.
General:
Accessibility:
Development:
Product Reviewer PR Checklist
Items the product team is responsible for reviewing.
General:
Accessibility:
Design Reviewer PR Checklist
Items the design team is responsible for reviewing.
General:
Developer Reviewer PR Checklist
Items the development team is responsible for reviewing.
General:
Accessibility: